Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

*: skip log timeout on maxExecutiontime as slow query for DDL statement. #38671

Merged
merged 6 commits into from
Oct 27, 2022
Merged

*: skip log timeout on maxExecutiontime as slow query for DDL statement. #38671

merged 6 commits into from
Oct 27, 2022

Conversation

Benjamin2037
Copy link
Collaborator

@Benjamin2037 Benjamin2037 commented Oct 27, 2022 •
edited
Loading

What problem does this PR solve?

When user issue an long run DDL statement, it should not be checked in maxExecutionTime for slow query checking mechanism.

Issue Number: close #38580

Problem Summary:
Just Skip none query type statements max execution time checking and kill them.

What is changed and how it works?

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code
None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Oct 27, 2022 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • hawkingrei
  • xhebox

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 27, 2022
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2022
@Benjamin2037 Benjamin2037 requested review from tiancaiamao, xhebox, mjonss, hawkingrei and xiongjiwei and removed request for mjonss October 27, 2022 01:24
xhebox
xhebox reviewed Oct 27, 2022
View reviewed changes
Copy link
Contributor

@xhebox xhebox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but we do have descriptions:

Unlike in MySQL, the max_execution_time system variable currently works on all kinds of statements in TiDB, not only restricted to the SELECT statement. The precision of the timeout value is roughly 100ms. This means the statement might not be terminated in accurate milliseconds as you specify.

https://docs.pingcap.com/tidb/dev/system-variables#max_execution_time

This should be investigated.

@Benjamin2037
Copy link
Collaborator Author

LGTM, but we do have descriptions:

Unlike in MySQL, the max_execution_time system variable currently works on all kinds of statements in TiDB, not only restricted to the SELECT statement. The precision of the timeout value is roughly 100ms. This means the statement might not be terminated in accurate milliseconds as you specify.

https://docs.pingcap.com/tidb/dev/system-variables#max_execution_time

This should be investigated.

This Note is not correct, currently we do not support max_execution_time on all kinds of statements in TiDB.

@xhebox
Copy link
Contributor

xhebox commented Oct 27, 2022

LGTM, but we do have descriptions:

Unlike in MySQL, the max_execution_time system variable currently works on all kinds of statements in TiDB, not only restricted to the SELECT statement. The precision of the timeout value is roughly 100ms. This means the statement might not be terminated in accurate milliseconds as you specify.

https://docs.pingcap.com/tidb/dev/system-variables#max_execution_time
This should be investigated.

This Note is not correct, currently we do not support max_execution_time on all kinds of statements in TiDB.

#10541 According to this PR, I think your argument makes more sense. Then https://github.com/pingcap/docs-cn/blob/master/develop/dev-guide-timeouts-in-tidb.md and its english version needs to be modified along this PR.

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 27, 2022
@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 27, 2022
@hawkingrei
Copy link
Member

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: f3d2466

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 27, 2022
@wuhuizuo
Copy link
Contributor

/run-build
/run-mysql-test

1 similar comment
@wuhuizuo
Copy link
Contributor

/run-build
/run-mysql-test

@ti-chi-bot ti-chi-bot merged commit bba321c into pingcap:master Oct 27, 2022
@sre-bot
Copy link
Contributor

sre-bot commented Oct 27, 2022

TiDB MergeCI notify

✅ Well Done! New fixed [1] after this pr merged.

CI Name Result Duration Compare with Parent commit
idc-jenkins-ci-tidb/integration-compatibility-test ✅ all 1 tests passed 2 min 34 sec Fixed
idc-jenkins-ci-tidb/integration-ddl-test 🟢 all 6 tests passed 28 min Existing passed
idc-jenkins-ci/integration-cdc-test 🟢 all 38 tests passed 21 min Existing passed
idc-jenkins-ci-tidb/integration-common-test 🟢 all 17 tests passed 10 min Existing passed
idc-jenkins-ci-tidb/common-test 🟢 all 11 tests passed 9 min 6 sec Existing passed
idc-jenkins-ci-tidb/tics-test 🟢 all 1 tests passed 5 min 46 sec Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2 🟢 all 28 tests passed 5 min 36 sec Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1 🟢 all 26 tests passed 4 min 54 sec Existing passed
idc-jenkins-ci-tidb/mybatis-test 🟢 all 1 tests passed 2 min 54 sec Existing passed
idc-jenkins-ci-tidb/plugin-test 🟢 build success, plugin test success 4min Existing passed

@Benjamin2037 Benjamin2037 deleted the maxExecutionTime_skip_kill_ddl branch October 28, 2022 01:10
This was referenced Dec 21, 2022
Closed
Closed
@Benjamin2037 Benjamin2037 mentioned this pull request Jun 19, 2023
Merged
17 tasks
@ran-huang ran-huang mentioned this pull request Jun 20, 2023
Merged
17 tasks
This was referenced Jun 20, 2023
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
@Defined2014 Defined2014 mentioned this pull request Oct 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hawkingrei hawkingrei hawkingrei approved these changes

@xhebox xhebox xhebox approved these changes

@tiancaiamao tiancaiamao Awaiting requested review from tiancaiamao

@xiongjiwei xiongjiwei Awaiting requested review from xiongjiwei

Assignees
No one assigned
Labels
release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Max execution time mechanism skip checking DDL jobs
6 participants
@Benjamin2037 @ti-chi-bot @xhebox @hawkingrei @wuhuizuo @sre-bot